-
Notifications
You must be signed in to change notification settings - Fork 9.4k
fix Can't remove samples link product downloadable #31895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.4-develop
Are you sure you want to change the base?
fix Can't remove samples link product downloadable #31895
Conversation
Hi @omaxmo. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @omaxmo, nice catch! Thanks for contributing. I believe now we need to update the unit test that is failing at the moment, and also, based on our definition of done, all contributions must be covered with automated tests. In this case the Unit test is not enough since it wasn't picking up the previous issue.
Please let me know if you need any help with it! Thanks!
@magento run Unit Tests |
@magento run all tests |
@magento run Integration Tests, Functional Tests EE, Functional Tests B2B |
@magento run all tests |
@magento run Integration Tests |
@magento run Integration Tests |
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
app/code/Magento/Downloadable/Test/Mftf/Test/AdminDeleteDownloadableProductSampleTest.xml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @engcom-Foxtrot, thanks for updating the PR
Can you check my comments below? Thanks!
@@ -326,10 +326,8 @@ public function testUpdateDownloadableProductData(): void | |||
$response = $this->saveProduct($productData); | |||
|
|||
$this->assertArrayHasKey(ProductInterface::EXTENSION_ATTRIBUTES_KEY, $response); | |||
$this->assertArrayHasKey(self::PRODUCT_SAMPLES, $response[ProductInterface::EXTENSION_ATTRIBUTES_KEY]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets double check if we need to remove these assertions
@@ -129,9 +129,6 @@ public function testExecuteNonDownloadable(): void | |||
$this->entityMock->expects($this->once()) | |||
->method('getTypeId') | |||
->willReturn(Type::TYPE_DOWNLOADABLE . 'some'); | |||
$this->entityMock->expects($this->once()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets double check if we need to remove these assertions
$this->assertArrayHasKey(self::PRODUCT_LINKS, $response[ProductInterface::EXTENSION_ATTRIBUTES_KEY]); | ||
|
||
$this->assertCount(2, $response[ProductInterface::EXTENSION_ATTRIBUTES_KEY][self::PRODUCT_SAMPLES]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets double check if we need to remove these assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @omaxmo, the proposed solution works, but it causes another problem on webapi interface.
With such fix, whenever a user updates a product via webapi its samples will be removed, this behavior doesn't happen outside the PR because the function updateSamples
doesn't get called if the $samples
is empty, but since that was removed, not it is getting called and ends up removing all samples. Can you check that?
Thanks!
@gabrieldagama I don't have any problems. Magento 2.4.2 Response:
All samples present after the update. |
Hi @omaxmo, I believe you should test with your PR, the problem is introduced with this change. Thanks! |
Hi @gabrieldagama , Yes, I know. But still don't have a problem. Do I need to update some specific attributes?
|
@gabrieldagama It's not working after merge. I will check |
…product-samples-delete-fix
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests CE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
As per this comment, tried to update the product "test" by webapi Response::
Observed that after updating the product via web API, the samples are not getting removed. Tested after latest Magento code. Also all the test cases are getting passed and the build is green, hence asking for review. Thank you! |
Description (*)
When you create a product downloadable and create samples link or simples file then you can't remove the all sample it. This error in Magento 2.4.1. It works fine in Magento 2.4.0
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)